-
Notifications
You must be signed in to change notification settings - Fork 178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: avoid diff in state after import for undefined optional attribute in alert config notification #1412
fix: avoid diff in state after import for undefined optional attribute in alert config notification #1412
Conversation
PlanModifiers: []planmodifier.Int64{ | ||
int64planmodifier.UseStateForUnknown(), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UseStateForUnknown(): Copies the prior state value, if not null. This is useful for reducing (known after apply) plan outputs for computed attributes which are known to not change over time.
Why is this modifier needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially to simplify the terraform plan output. The interval_min
attribute will only change its value when modified by the user.
As a counter example, alert configuration has an updated
attribute that is computed and is not set with this modifier. In this case is seems appropriate to have the message (known after apply)
shown on the terraform plan because on every apply it will be modified.
if !currState.IntervalMin.IsNull() { | ||
newState.IntervalMin = types.Int64Value(int64(value.IntervalMin)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was reading this plan-modification#plan-modification-process
Plan Modification Process
When the provider receives a request to generate the plan for a resource change via the framework, the following occurs:Set any attributes with a null configuration value to the default value.
If the plan differs from the current resource state, the framework marks computed attributes that are null in the configuration as unknown in the plan. This is intended to prevent unexpected Terraform errors. Providers can later enter any values that may be known.
Run attribute plan modifiers.
Run resource plan modifiers.
I am wondering if currState.IntervalMin.isNill
is false
but currState.IntervalMin.IsUnknown()
is true
when calling terraform plan
. Could you verify? if that is the case we should add currState.IntervalMin.IsUnknown()
in all of this ifs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes your statement is correct, I could verify that the attribute in currState
comes as unknown as it is an undefined computed attribute.
In this case, since it is a value that is always defined in the api request/response (defined as an int in manual sdk), we can simply define the value in the model. This is what solves the inconsistency issue when doing the import operation, because when importing you have no state/config to verify if field was defined by the user, so value coming from the api is set directly.
…e (alert_configuration.notification.interval_min)
00d5bfb
to
3752da6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the discussion
@maastha small change here just in case you want to take a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Related ticket: case found when testing INTMDB-981
Summary: adjust optional attribute
interval_min
to handle default values the same way asdelay_min
to avoid non empty plan after using import operation.For the following valid resource that does not define
interval_min
attribute:When import statement and HCL code is generated using the following data source for the created resource above:
[Previous] Terraform plan output on the generated resource code results in a non empty plan:
[New] Terraform plan output:
Type of change:
Required Checklist:
Further comments
Test that was defined needs credentials, so ran all resource and data source tests: